VPAAMP-346 restore period state when UpdateTrackInfo fails in HandleS…#1454
Open
pstroffolino wants to merge 2 commits into
Open
VPAAMP-346 restore period state when UpdateTrackInfo fails in HandleS…#1454pstroffolino wants to merge 2 commits into
pstroffolino wants to merge 2 commits into
Conversation
…eekEOSAndPeriodTransition HandleSeekEOSAndPeriodTransition updates six period-identity members (mCurrentPeriodIdx, mCurrentPeriod, mBasePeriodId, mPeriodStartTime, mPeriodDuration, mPeriodEndTime) before calling UpdateTrackInfo. If UpdateTrackInfo returns failure (malformed period, no codec-compatible representation, null track context after live manifest refresh), the function returned false but left all six members pointing at the new, uninitialised period. The next fetcher-loop iteration would read mCurrentPeriodIdx as the new value and attempt to download fragments from a period whose tracks were never set up, producing download errors or incorrect init-segment selection. Fix: snapshot all six members before the switch. On UpdateTrackInfo failure, restore them and return false so the caller sees a clean no-op. Unit test: HandleSeekEOS_UpdateTrackInfoFails_PeriodStateRestored forces UpdateTrackInfo to fail by nulling the audio track context (UpdateTrackInfo returns MANIFEST_CONTENT_ERROR on a null context) and verifies that mCurrentPeriodIdx is restored to its pre-switch value.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to make MPD period transitions safer by rolling back period state when HandleSeekEOSAndPeriodTransition() cannot complete UpdateTrackInfo(), preventing the fetch loop from continuing with a partially switched period.
Changes:
- Adds period-state snapshot/restore logic around the period switch path.
- Adds an L1 regression test that forces
UpdateTrackInfo()failure during a forward period transition.
L1 test verdict: Mostly valid, but needs revision.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
fragmentcollector_mpd.cpp |
Restores selected period identity fields when UpdateTrackInfo() fails during EOS-driven period transition. |
test/utests/tests/StreamAbstractionAAMP_MPD/FetcherLoopTests.cpp |
Adds test accessors and a regression test for failed period-switch rollback. |
Comment on lines
+1993
to
+1998
| mCurrentPeriodIdx = savedPeriodIdx; | ||
| mCurrentPeriod = savedCurrentPeriod; | ||
| mBasePeriodId = savedBasePeriodId; | ||
| mPeriodStartTime = savedPeriodStartTime; | ||
| mPeriodDuration = savedPeriodDuration; | ||
| mPeriodEndTime = savedPeriodEndTime; |
|
|
||
| // UpdateTrackInfo failed: the period switch must have been rolled back. | ||
| EXPECT_FALSE(transitioned); | ||
| EXPECT_EQ(mTestableStreamAbstractionAAMP_MPD->GetCurrentPeriodIdx(), periodBefore); |
…AndPeriodTransition StreamSelection() and UpdateTrackInfo() mutate per-track state (adaptationSet, representation, fragmentDescriptor.Number/Time/Bandwidth, eos, timeLineIndex, fragmentRepeatCount, scaledPTO, enabled, adaptationSetIdx, adaptationSetId, representationIndex, profileChanged) one track at a time. The previous rollback only restored the six period-identity scalars (mCurrentPeriodIdx, mCurrentPeriod, mBasePeriodId, mPeriodStartTime, mPeriodDuration, mPeriodEndTime). If UpdateTrackInfo() failed after processing video (track 0) but before processing audio (track 1), the video context was left pointing at the new period's adaptation set and representation while the period index was restored to the old period, putting the object in a state that would cause wrong-period fragment downloads on the next fetcher-loop iteration. Fix: snapshot all mutable per-track fields (via a local TrackSnapshot struct across all mMaxTracks slots) plus mNumberOfTracks, mUpdateStreamInfo, mABRMode, mStreamInfo, and mBitrateIndexVector before calling StreamSelection()/ UpdateTrackInfo(). Restore the full snapshot on UpdateTrackInfo() failure so the object is in exactly the same state as before the attempted transition. test: add HandleSeekEOS_UpdateTrackInfoFails_PeriodStateRestored assertions The existing test only checked mCurrentPeriodIdx and the boolean return value. Added four accessors to TestableStreamAbstractionAAMP_MPD (GetBasePeriodId, GetPeriodStartTime, GetPeriodDuration, GetPeriodEndTime) and extended the test to snapshot and re-assert every period-identity field plus the video track's adaptationSet pointer, representation pointer, fragmentDescriptor.Number, and eos flag after the failed transition, so an incomplete rollback that restores only the index would cause the test to fail.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…eekEOSAndPeriodTransition
HandleSeekEOSAndPeriodTransition updates six period-identity members (mCurrentPeriodIdx, mCurrentPeriod, mBasePeriodId, mPeriodStartTime, mPeriodDuration, mPeriodEndTime) before calling UpdateTrackInfo. If UpdateTrackInfo returns failure (malformed period, no codec-compatible representation, null track context after live manifest refresh), the function returned false but left all six members pointing at the new, uninitialised period. The next fetcher-loop iteration would read mCurrentPeriodIdx as the new value and attempt to download fragments from a period whose tracks were never set up, producing download errors or incorrect init-segment selection.
Fix: snapshot all six members before the switch. On UpdateTrackInfo failure, restore them and return false so the caller sees a clean no-op.
Unit test: HandleSeekEOS_UpdateTrackInfoFails_PeriodStateRestored forces UpdateTrackInfo to fail by nulling the audio track context (UpdateTrackInfo returns MANIFEST_CONTENT_ERROR on a null context) and verifies that mCurrentPeriodIdx is restored to its pre-switch value.